refactor: reduce duplication and simplify codebase#374
Conversation
Eliminate copy-pasted functions, inline error boilerplate, and repeated patterns across daemon handlers and platform modules. Key changes: - Deduplicate isEnvTruthy, displayNodeLabel, roundPercent into single sources - Extract throwDaemonError helper for client/CLI daemon response errors - Add sessionNotFoundResponse/unsupportedOperationResponse helpers and adopt errorResponse() across ~30 handler files (-565 lines) - Unify BATCH_PARENT_FLAG_KEYS/REPLAY_PARENT_FLAG_KEYS into shared mergeParentFlags helper in handler-utils - Extract createLinuxToolResolver for screenshot/clipboard tool detection - Remove stale type-sync comments from contracts.ts and metro.ts
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4814eb7808
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import { AppError } from './errors.ts'; | ||
| import type { CliFlags } from './command-schema.ts'; | ||
| import type { DaemonLockPolicy } from '../daemon/types.ts'; | ||
| import { isEnvTruthy } from './retry.ts'; |
There was a problem hiding this comment.
Switching session-binding.ts to import isEnvTruthy from retry.ts changes behavior for AGENT_DEVICE_SESSION_LOCKED: the previous local parser used raw.trim().toLowerCase(), but the shared helper only lowercases. Values like ' true ' or ' yes ' now evaluate as false, so resolveLockMode can silently skip the intended 'reject' lock policy in padded/templated env setups that previously worked.
Useful? React with 👍 / 👎.
Bigger wins from restructuring, not just mechanical dedup: - cli.ts: move logTailStopper to try/finally (eliminates 28 duplicate calls), extract writeCommandCliOutput/writeLogsCliOutput/writeNetworkCliOutput from 350-line if/else chain, fix remaining throwDaemonError site - session-store.ts: replace 67-line sanitizeFlags destructure/reconstruct with 10-line pick-from-array loop - record-trace: extract finalizeRecordingOverlay helper, replacing 4 copies of the telemetry+overlay block across ios/android/recording files - Deduplicate normalizeText (finders.ts + selectors-match.ts) - Fix isEnvTruthy to preserve whitespace-tolerant parsing (.trim())
process.exit() does not unwind the stack, so finally blocks are skipped. Restore explicit logTailStopper() calls before each process.exit() to prevent leaking the background daemon log tail process. The finally block remains as a safety net for normal return paths. Also refactor writeCommandCliOutput to return an exit code instead of calling process.exit() directly, keeping the exit decision in the caller where cleanup is visible.
1 participant
Summary
Two-pass refactoring to eliminate unnecessary duplication and simplify the codebase. Net result: -751 lines across 55 files with zero test regressions (all 1237 tests pass).
Pass 1: Mechanical deduplication
Deduplicated identical functions:
isEnvTruthy()— was in bothretry.tsandsession-binding.ts(with .trim() preserved)displayNodeLabel()— was in bothoutput.tsandmobile-snapshot-semantics.tsroundPercent()— was in bothandroid/perf.tsandios/perf.tsnormalizeText()— was in bothfinders.tsandselectors-match.tsclient.tsandcli.tsStandardized error responses across ~30 handler files:
sessionNotFoundResponse()andunsupportedOperationResponse(command)helpers{ ok: false, error: { code, message } }toerrorResponse()Unified handler boilerplate:
mergeParentFlags()replacing identical arrays in batch/replayLinux tool resolver:
createLinuxToolResolver<T>()replacing copy-pasted detection+caching in screenshot/clipboardPass 2: Structural simplification
CLI output dispatch (-250 lines):
logTailStoppercleanup intotry/finally— eliminates 28 duplicate guard callswriteCommandCliOutput(),writeLogsCliOutput(),writeNetworkCliOutput()from 350-line if/else chainsanitizeFlags (-57 lines):
Recording finalization (-45 lines):
finalizeRecordingOverlay()replacing 4 copies of telemetry+overlay block across record-trace-ios/android/recordingNew files (4)
src/daemon-error.ts—throwDaemonError()for client-side daemon response errorssrc/platforms/perf-utils.ts—roundPercent()shared by iOS and Android perfsrc/platforms/linux/tool-resolver.ts— generic Linux tool resolution with cachingsrc/daemon/handlers/record-trace-finalize.ts— shared recording overlay finalizationTest plan
npx tsc --noEmit— zero type errorsnpm test— all 1237 tests pass (both passes)isEnvTruthypreserves.trim()for whitespace-tolerant env parsing